-
Notifications
You must be signed in to change notification settings - Fork 2
Update dev tooling #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Outstanding issues (I need feedback):
|
| ### Required software | ||
|
|
||
| - **Python 3.11 or 3.12** - The demo requires a recent Python version | ||
| - **Python 3.10, 3.11 or 3.12** - The demo requires a recent Python version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryanmerolle there was some reason I went with >3.10, but can't remember which part of this bundle cared about that. Did you test this on 3.10?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I just noticed pyproject.toml mentioned requires-python = ">=3.10,<3.13" so I was updating to match that. I can test if you need, or we can just bump everything to 3.11 and not think twice about this given uv and venv.
|
|
||
| # 5. Populate security relationships (optional, required if you loaded security data) | ||
| uv run python scripts/populate_security_relationships.py | ||
| # 5. Add the demo repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure why you're recommending that the firewall policy relationships don't get loaded. Otherwise the firewall rules aren't all linked. This is a workaround to Infrahub object files not being able to support many-to-many relationships at present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not recommending that. I noticed there is no file with that name in the repo.
➜ infrahub-bundle-dc git:(repo_tooling) ✗ tree scripts
scripts
├── bootstrap.py
├── create_proposed_change.py
├── create_users_roles.py
└── get_configs.py
1 directory, 4 files
➜ infrahub-bundle-dc git:(repo_tooling) ✗ find . -name "populate_security_relationships.py"
➜ infrahub-bundle-dc git:(repo_tooling) ✗ find . -name "create_proposed_change.py"
./scripts/create_proposed_change.py| ### Documentation | ||
|
|
||
| For detailed setup instructions, configuration options, and usage guide, see [SERVICE_CATALOG.md](SERVICE_CATALOG.md). | ||
| For detailed setup instructions, configuration options, and usage guide, see [docs/docs/service-catalog.mdx](docs/docs/service-catalog.mdx). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the correct path here should be ./service-catalog.mdx as it's a file in the same directory, otherwise it'll look for docs/docs/docs/docs/service-catalog.mdx`
I think the previous one worked because docusaurus strips off .md or .mdx when compiling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the root level README.md. If you click the link from the README.md from this branch you will see it works and redirects you appropriately,
|
I think the challenge here is your tooling is different to ours, and as we're trying to run consistent tooling across many employees, many repos and many CI pipelines so that it's more maintainable when they're as consistent as possible. It would be good to pull apart the content differences vs. the linting/tooling differences. |
Always a fair comment. I think its pretty safe to say the following are non-controversial an use your same tooling:
I am not aware of any tooling for jinja that would conflict with implementing the j2linter. It makes Jinja2 way more readable. It's developed and used by Arista for that end on the AVD repo. It's slowly made its way into some popular projects associated with Ansible, Openstack, etc. prek/pre-commit is an optional thing. I am happy to remove it. Tell me what you like and what you want ripped out and I'm happy to adjust. |
8e914ff to
dab3b46
Compare
Uh oh!
There was an error while loading. Please reload this page.